Replace Websockets with GRPC #20

webwarrior-ws · 2024-02-06T13:29:02Z

Fixes https://github.com/nodeeffect/backlog/issues/4

src/FX.GrpcService/Services/FXService.cs

src/FX.Tests/E2ETests.cs

src/FX.GrpcService/Services/FXService.cs

src/FX.Tests/E2ETests.cs

src/FX.GrpcClient/Instance.cs

.github/workflows/CI.yml

src/FX.GrpcService/Services/FXService.cs

knocte · 2024-02-08T06:22:57Z

@webwarrior-ws please split the e2e commit in two: first commit will work with normal HTTP, 2nd would change it to HTTPS

webwarrior-ws · 2024-02-08T09:52:12Z

@webwarrior-ws please split the e2e commit in two: first commit will work with normal HTTP, 2nd would change it to HTTPS

It doesn't work with HTTP:

Grpc.Core.RpcException : Status(StatusCode="Unavailable", Detail="Error starting gRPC call. HttpRequestException: An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake. (InvalidResponse) HttpIOException: An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake. (InvalidResponse) HttpIOException: The response ended prematurely while waiting for the next frame from the server. (ResponseEnded)", DebugException="System.Net.Http.HttpRequestException: An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake. (InvalidResponse)

knocte · 2024-02-08T09:57:39Z

It doesn't work with HTTP:

It works in RunIntoMe. In fact it's what we have in production.

webwarrior-ws · 2024-02-08T10:27:27Z

Got it to work with HTTP. Split commit into 2.

knocte · 2024-02-08T11:09:51Z

CI of 1st commit is red, please fix; after that I'll merge

webwarrior-ws · 2024-02-08T11:29:52Z

CI for all commits is green now.

knocte · 2024-02-08T11:37:12Z

I just realized that the WebSocket proj already had the Core project hooked up (see src/WebSocketApp/Models.fs), so:

In the last commit, let's move the warningsAsError settings to Directory.Build.Props file so that it applies to all projs.
Let's add one more extra commit that hooks to same operations as we had in src/WebSocketApp/Models.fs. For this you might need a new project (not 100% sure) shared between GrpcClient and GrpcService, so that they can serialize/deserialize to/from JSON to extract/encode request details.

webwarrior-ws · 2024-02-08T13:34:13Z

@knocte I added models and client/server interop.

knocte · 2024-02-08T13:43:46Z

@webwarrior-ws great, but you didn't move WarningsAsErrors yet

webwarrior-ws · 2024-02-08T13:49:47Z

Moved TreatWarningsAsErrors to Directory.Build.props.

Directory.Build.props

src/FX.GrpcModels/Models.fs

src/FX.GrpcService/Services/FXService.cs

src/FX.GrpcClient/Instance.cs

src/FX.Tests/E2ETests.cs

src/FX.GrpcModels/Marshalling.fs

knocte · 2024-09-05T08:25:40Z

I have concerns with 2 of the commits you pushed yesterday:

Tests: clean DB after run in tests: why is its CI red?
Core,Grpc.Service: make SendMarketOrder return: this is wrong, market order can never match partially (unless a liquidity problem is found, in which case an exception will be raised; that's why it returns unit)

webwarrior-ws · 2024-09-05T08:30:57Z

Tests: clean DB after run in tests: why is its CI red?

Because old version of test expected first LimitOrder call to be a full match. Which was wrong, but worked due to not cleaning DB.

knocte · 2024-09-05T08:33:53Z

Because old version of test expected first LimitOrder call to be a full match. Which was wrong, but worked due to not cleaning DB.

Are you saying that these cleanup exposed a bug? Then explain in commit msg this fact so that it is understood why CI is red -> because CI is catching the bug that will be fixed later?

knocte · 2024-09-05T08:58:14Z

Last but not least, these 3 commits:

GrpcModels,Core: move Match converter -> can be squashed to the commit that introduces this converter for the first time
Directory.Packages.props: update STJ -> can be squashed? if this is already in master, then put this as the 1st commit of the PR please
Core: fix typo in MatchTypeConverter -> can be squashed to the commit that introduces this converter for the first time

webwarrior-ws · 2024-09-05T09:04:27Z

Directory.Packages.props: update STJ -> can be squashed? if this is already in master, then put this as the 1st commit of the PR please

I don't see any commit to squash this with. It is not connected to any other work in this PR, but is necessary for CI to succeed now as version 8.0.1 got marked as vulnerable.

knocte · 2024-09-05T09:05:42Z

I don't see any commit to squash this with. It is not connected to any other work in this PR, but is necessary for CI to succeed now as version 8.0.1 got marked as vulnerable.

Read the message again, I gave a hint of what to do in that case.

webwarrior-ws · 2024-09-05T09:08:18Z

You only said what to do if that commit is in master (it is not).

knocte · 2024-09-05T09:09:22Z

I meant if STJ is already in master.

Update System.Text.Json library to version 8.0.4 since previously used version (8.0.1) contains vlunerabilities.

Removed WebSocket project and added GRPC projects from RIM (copy&paste). Then removed unused stuff from GRPC* projects and specified package versions in Directory.Packages.props.

Made end-to-end GRPC test that makes sure GRPC communication is working.

Use HTTPS connection for GRPC communtication. Also added commands to CI that trust ASP.NET certificate on Linux because otherwise GRPC communication fails.

Enable TreatWarningsAsErrors in Directory.Build.props. Exception is CS8981 (lowercase type names) in GrpcClient and GrpcService projects because it is triggered by auto-generated protobuf types.

Added GrpcModels project for server/client interop which includes message models (that were in WebSocketApp), as well as Marshalling module (code from RIM). Use serialization options defined in Core/RedisStorageLayer.fs in Marshalling module. This is needed for F# types support.

knocte · 2024-09-05T10:13:42Z

Last but not least, these 3 commits:

Only one of these has been addressed.

Implemented json converters for Match and Option<'T> types. Match type converter is placed in GrpcModels.ModelSerialization since it's not used in Redis, but only in GRPC.

Modified e2e test to use message that returns response so that serilization is tested.

Clean DB after each test run in BasicTests, MakerOnlyOrders, MarketOrders, and LimitOrders test classes. This is needed for all tests to start from empty state. This commit exposes bug in end2end test - response to the first LimitOrder should be None, not Some(Full). It will be fixed later.

Expand GrpcE2ETest so that Match.Partial case is used and any serialization errors cancerning it will be caught by this test.

Fix in Match type deserialization (Partial case).

webwarrior-ws · 2024-09-05T10:21:18Z

Squashed json converters commits